Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Fake Analytics instance that uses list to save events sent #149

Merged
merged 7 commits into from
Sep 13, 2023

Conversation

eliasyishak
Copy link
Contributor

@eliasyishak eliasyishak commented Aug 22, 2023

Address issue:

This PR adds a subclass of Analytics that extends the default AnalyticsImpl implementation and overrides the .send(Event) method.


  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

/// Use this list to check for events that have been emitted when
/// invoking the send method
final List<Event> sentEvents = [];

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bkonyi thoughts on this so far. I figured that for the fake instance, all you really need is to have access to a list that will contain each Event we send so we only really need to override the send method to add to the list

I think all other functionality within the default implementation class should be fine?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think this looks good to me! Assuming each Event implementation has operator == and hashCode overrides, this will work perfectly :).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah good call!

Comment on lines +387 to +395
@override
int get hashCode => eventData.hashCode;

@override
bool operator ==(Object other) =>
other is Event &&
other.runtimeType == runtimeType &&
other.eventName == eventName &&
compareEventData(other.eventData, eventData);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added these overrides with some tests to ensure it works correctly, but does this look good? @bkonyi

We essentially just need to check the event name and the data in the eventData map

@eliasyishak eliasyishak marked this pull request as ready for review September 12, 2023 22:51
@eliasyishak
Copy link
Contributor Author

@bkonyi friendly ping, lmk if you think this is good to go

@eliasyishak
Copy link
Contributor Author

@christopherfujino requesting since Ben is OOO until next week, this PR looks to add a fake instance we can ship out to clients so that they can keep track of events in a list for testing purposes

@@ -627,6 +627,50 @@ class AnalyticsImpl implements Analytics {
}
}

class FakeAnalytics extends AnalyticsImpl {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should have a dartdoc explaining exactly what is different between this and AnalyticsImpl.

Copy link
Member

@christopherfujino christopherfujino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

other than a nit about the dartdoc, this LGTM

@eliasyishak eliasyishak merged commit 1512f3d into dart-lang:main Sep 13, 2023
@eliasyishak eliasyishak deleted the 137-ship-mock-analytics branch September 13, 2023 18:12
Copy link
Contributor

@bkonyi bkonyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay, but this LGTM!

mosuem pushed a commit that referenced this pull request Aug 13, 2024
* support 'package:http' 2.0.0

* update changelog

* update the CI SDK versions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants